Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sdk 331 account management should be moved to instruments #246

Conversation

Nagaprasadvr
Copy link
Contributor

  1. Added few more methods to LegInstrument Insterface like getBaseAssetMints, getOracleAccounts etc
  2. cleaned up and removed helper functions associated with rfq legs

@changeset-bot
Copy link

changeset-bot bot commented Sep 25, 2023

⚠️ No Changeset found

Latest commit: 2b58be9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@linear
Copy link

linear bot commented Sep 25, 2023

SDK-331 Account management should be moved to instruments

these clog up handlers. Should add these to instrument. Should not construct accounts directly in operations

const optionMarketTxBuilder =
TransactionBuilder.make().setFeePayer(payer);
if (optionMarketIxs.length > 0) {
optionMarketIxs.forEach((ix) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 2 locations. Consider refactoring.

if (optionMarketIxs.length === 0) continue;
const optionMarketTxBuilder =
TransactionBuilder.make().setFeePayer(payer);
optionMarketIxs.forEach((ix) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 2 locations. Consider refactoring.

}
const flattendedTransactions = transactionMatrix.flat();
const { keypairs, identities } = getSignerHistogram(signers);
for (let transaction of flattendedTransactions) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 2 locations. Consider refactoring.

);
return optionMarketIxs;
}
getBaseAssetAccount(): AccountMeta {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

async getPreparationsBeforeRfqCreation(): Promise<CreateOptionInstrumentsResult> {
return [];
}
getBaseAssetAccount(): AccountMeta {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

};
}

getBaseAssetAccount(): AccountMeta {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

getDecimals = () => PsyoptionsAmericanInstrument.decimals;
getSide = () => this.side;
toLeg(): Leg {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

};
return oracleAccount;
}
toLeg(): Leg {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.


return optionMarketIxs;
}
toLeg(): Leg {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

.map((ins) => ins.getValidationAccounts())
.flat();
baseAssetAccounts = instrumentsToAdd.map((i) => i.getBaseAssetAccount());
rfqBuilder = TransactionBuilder.make()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 2 locations. Consider refactoring.


return TransactionBuilder.make()
let rfqBuilder = TransactionBuilder.make()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 2 locations. Consider refactoring.

return this.underlyingAssetMint;
}

async getOracleAccount(): Promise<AccountMeta> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

return this.mintAddress;
}

async getOracleAccount(): Promise<AccountMeta> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

return this.underlyingAssetMint;
}

async getOracleAccount(): Promise<AccountMeta> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

@@ -254,3 +368,157 @@ export const createAmericanProgram = (

return americanProgram;
};

export const getPsyAmericanMarketIxs = async (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function getPsyAmericanMarketIxs has 44 lines of code (exceeds 25 allowed). Consider refactoring.

@Nagaprasadvr Nagaprasadvr changed the base branch from dev to sdk-366-create-option-markets-and-ata-in-createrfq-if-the-options September 25, 2023 03:50
@Nagaprasadvr Nagaprasadvr changed the base branch from sdk-366-create-option-markets-and-ata-in-createrfq-if-the-options to dev September 25, 2023 03:52
@Nagaprasadvr Nagaprasadvr changed the base branch from dev to sdk-366-create-option-markets-and-ata-in-createrfq-if-the-options September 25, 2023 03:57
@@ -1,4 +1,8 @@
import { PublicKey, TransactionInstruction } from '@solana/web3.js';
import {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File instrument.ts has 447 lines of code (exceeds 250 allowed). Consider refactoring.

@@ -1,4 +1,9 @@
import { Keypair, PublicKey, TransactionInstruction } from '@solana/web3.js';
import {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File instrument.ts has 473 lines of code (exceeds 250 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Sep 25, 2023

Code Climate has analyzed commit 4383e1c and detected 31 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 12
Duplication 19

View more on Code Climate.

instrumentDecimals: legInstrument.getDecimals(),
side: toSolitaLegSide(legInstrument.getSide()),
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why to move this logic to the instrument interface? The logic in each instrument is the same and is a clone of this logic

getValidationAccounts(): AccountMeta[];
getPreparationsBeforeRfqCreation(): Promise<CreateOptionInstrumentsResult>;
getUnderlyingBaseAssetMint(): PublicKey;
toLeg(): Leg;
Copy link
Contributor

@EquilateralDelta EquilateralDelta Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of those method are option-specific, but not generic. For example, they can be confusing when applied to spot instrument, rename getUnderlyingBaseAssetMint

@pindaroso pindaroso closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants